aws: catch auth errors on XML responses#11891
Open
antoniomrfranco wants to merge 1 commit into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughConsolidates XML auth-error checks into one strcasestr() match across more AWS XML identifiers and extends JSON error-code recognition to include InvalidSecurity, TokenRefreshRequired, and InvalidSignature in flb_aws_is_auth_error(). ChangesExpanded auth error detection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/aws/flb_aws_util.csrc/aws/flb_aws_util.c:20:10: fatal error: 'fluent-bit/flb_info.h' file not found ... [truncated 727 characters] ... /infer-linux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/lib/clang/18/include" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Author
|
Debug log output from testing the change: |
Author
|
Valgrind output that shows no leaks or memory corruption was found: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aws/flb_aws_util.c`:
- Around line 349-360: Fix spacing and indentation in the AWS XML error-check
block: add a space before "!= NULL" in the "SignatureDoesNotMatch" check so it
reads like the other strcasestr comparisons, and adjust the indentation of the
"return FLB_TRUE;" line to align with the other conditional body lines (use the
same 8-space indentation as the surrounding block); locate the block where
strcasestr(payload, "SignatureDoesNotMatch") is checked and the subsequent
return to make these two small edits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
antoniomrfranco
force-pushed
the
fix-s3-auth-error-cred-refresh
branch
from
a3a9820 to
7ebc6f8
Compare
Signed-off-by: Antônio Franco <13881523+antoniomrfranco@users.noreply.github.com>
antoniomrfranco
force-pushed
the
fix-s3-auth-error-cred-refresh
branch
from
7ebc6f8 to
53f3aed
Compare
antoniomrfranco
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When AWS credentials are rotated (e.g. a new key pair written to
~/.aws/credentials), the S3 output plugin fails to detect the authentication error and trigger a credential refresh.The root cause is in
flb_aws_is_auth_error()(src/aws/flb_aws_util.c): S3 returns error responses in XML format, but several auth-related error codes —InvalidAccessKeyId,SignatureDoesNotMatch,InvalidToken,InvalidSecurity,TokenRefreshRequired, andInvalidSignature— were only checked via the JSON path (usingflb_aws_error(), which parses the__typefield). The XML path only coveredInvalidClientTokenId,AccessDenied, andExpired.As a result,
flb_aws_client_request()never calledprovider->refresh()on S3 auth failures, so stale credentials were used indefinitely despite an updated credentials file on disk.Fix
The missing error codes are added to the XML
strcasestrcheck block inflb_aws_is_auth_error(). Whenflb_aws_client_request()receives a 4xx response, it callsflb_aws_is_auth_error()on the response body. If the function returnsFLB_TRUE, the client callsprovider->refresh(), which re-reads the credentials file from disk and picks up the rotated keys. The next upload attempt — triggered by the S3 timer callback — then uses the fresh credentials and succeeds. With this fix, that detection now works correctly for S3's XML responses, not just JSON-based APIs like Kinesis.Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit